Add spoly_is_convex #43

ggnmstr · 2023-08-02T17:58:46Z

Adds function to determine whether the spoly is convex.
Algorithm based on
https://mathoverflow.net/questions/193569/determining-orientation-of-spherical-polygons

vitcpp

In general everything is ok except of some trivial issues.

src/polygon.c

vitcpp · 2023-08-03T13:02:49Z

@ggnmstr thank you for the PR. I think everything is ok except of some trivial issues. The tests should be fixed - could you please try to run tests without healpix:

gmake USE_HEALPIX=0 install
gmake USE_HEALPIX=0 test

I would also ask to start the commit comment from capital lettet as it was done for other commits.

dura0ok · 2023-08-04T05:22:12Z

fix_tests.patch
Please @ggnmstr rename it to .patch extension and apply it! This fix your tests!!

ggnmstr · 2023-08-04T05:46:32Z

@stepan-neretin7 Thank you, it works, seems like there's a problem with healpix on my computer.

dura0ok · 2023-08-04T20:28:56Z

@ggnmstr please, start commit with capital letter

esabol · 2023-08-04T20:54:52Z

@stepan-neretin7 wrote:

@ggnmstr please, start commit with capital letter

Do you mean the title of the pull request, @stepan-neretin7 ? The one commit message at https://github.com/postgrespro/pgsphere/pull/43/commits does start with a capital letter.

dura0ok · 2023-08-04T21:49:24Z

@stepan-neretin7 wrote:

@ggnmstr please, start commit with capital letter

Do you mean the title of the pull request, @stepan-neretin7 ? The one commit message at https://github.com/postgrespro/pgsphere/pull/43/commits does start with a capital letter.

Yes, I think it's a good idea. I also suggest that you also specify the issue number in the comment, so that he automatically linked it to the desired issue even before the merge.

vitcpp · 2023-08-05T10:56:25Z

@ggnmstr There are some conflicts. Could you please do rebase on the newest version and fix the conflicts?

esabol · 2023-08-05T19:42:23Z

@ggnmstr: CI failures. Please check and fix "make test" results.

https://app.travis-ci.com/github/postgrespro/pgsphere/jobs/607409643

esabol · 2023-08-06T05:06:44Z

expected/init_test.out.in

+psql:pg_sphere.test.sql:8594: NOTICE:  argument type pointkey is only a shell
+psql:pg_sphere.test.sql:8600: NOTICE:  argument type pointkey is only a shell
+psql:pg_sphere.test.sql:9167: NOTICE:  return type smoc is only a shell
+psql:pg_sphere.test.sql:9173: NOTICE:  argument type smoc is only a shell


Add linefeed to the end of this file.

esabol · 2023-08-06T05:07:21Z

expected/init_test_healpix.out.in

-psql:pg_sphere.test.sql:9181: NOTICE:  return type smoc is only a shell
-psql:pg_sphere.test.sql:9187: NOTICE:  argument type smoc is only a shell
+psql:pg_sphere.test.sql:9167: NOTICE:  return type smoc is only a shell
+psql:pg_sphere.test.sql:9173: NOTICE:  argument type smoc is only a shell


Add a linefeed to the end of this file.

esabol · 2023-08-06T14:55:06Z

expected/init_test_healpix.out.in

-psql:pg_sphere.test.sql:9181: NOTICE:  return type smoc is only a shell
-psql:pg_sphere.test.sql:9187: NOTICE:  argument type smoc is only a shell
+psql:pg_sphere.test.sql:9167: NOTICE:  return type smoc is only a shell
+psql:pg_sphere.test.sql:9173: NOTICE:  argument type smoc is only a shell


You are almost there! Change "9167" to "9195" on line 1. Change "9173" to "9201" on line 2.

vitcpp · 2023-08-07T13:51:19Z

To finalize PR the following changes are required:

Update pg_sphere--1.2.3--1.3.0.sql. Add command to create the implemented function.
Add new function description into Functions -> spoly functions section of the User Manual (gmake pdf).

ggnmstr · 2023-08-08T03:12:35Z

@stepan-neretin7, this "return null" behavior is common among all functions, I don't think it is possible to change something just in place.

dura0ok · 2023-08-08T03:31:39Z

@stepan-neretin7, this "return null" behavior is common among all functions, I don't think it is possible to change something just in place.

but in your code you return false.. Very strange for me..

esabol

A couple more minor tweaks, please, @ggnmstr. Thank you!

esabol · 2023-08-08T05:42:25Z

src/polygon.c

+				wsv,
+				crs;
+	float8		cur = 0,
+				prev = 0;


Do the variables on lines 1487-1493 line up vertically for you? They don't line up vertically in the diff output here on GitHub.com.

Yes, they do.

The default tab size on github is 8. It can be changes to 4 in Profile -> Settings -> Appearance -> Tab size... Not sure, how to change it for a specific repository and specific file type.

I changed it. For me it looks like this:

esabol · 2023-08-08T05:44:11Z

src/polygon.h

+/*
+ * Checks whether a polygon is convex
+ */
+Datum spherepoly_is_convex(PG_FUNCTION_ARGS);


Add spaces after Datum so that spherepoly_is_convex lines up vertically with the names of the functions above this.

esabol · 2023-08-08T05:47:15Z

doc/functions.sgm

+              Spoly is convex
+            </title>
+            <para>
+              Returns true if the specified spherical polygon is convex. Returns false otherwise.


Is this line longer than 79 characters? I can't tell looking at it in the web browser here. If it is, please word-wrap it.

vitcpp · 2023-08-08T08:49:01Z

SELECT spoly_is_convex(NULL);

spoly_is_convex
(1 row)

why you return null? is this correct for interface?

I agree, it should return false. The third value is unnecessary. To fix it STRICT keyword should be removed in CREATE FUNCTION command. I also propose to update the tests by adding this case.

ggnmstr · 2023-08-08T09:45:01Z

@vitcpp. I fixed it and added a test, thanks for the information.

dura0ok · 2023-08-08T10:54:31Z

for (int i = 0; i < poly->npts; i++)
please move the variable initialization to the top of the function
@ggnmstr

vitcpp

spherepoly_is_convex() functions seems to be ok as the first working version, but the code is not optimized. There are duplicate calculations in the for loop that can slowdown the performance.

vitcpp · 2023-08-09T06:38:42Z

There are some conflicts that should be resolved. @ggnmstr Could you please rebase your branch to the latest postgrespro/pgsphere version and fix conflicts?

ggnmstr · 2023-08-09T07:19:34Z

spherepoly_is_convex() functions seems to be ok as the first working version, but the code is not optimized. There are duplicate calculations in the for loop that can slowdown the performance.

Yes, I agree with that. I think I'll optimize it a bit later.

vitcpp · 2023-08-10T11:41:39Z

upgrade_scripts/pg_sphere--1.2.3--1.3.0.sql.in

@@ -28,6 +28,7 @@ COMMENT ON FUNCTION spoly_deg(float8[]) IS
   refer to the same occurrence and cover its
   latitude and longitude, respectively.';

+<<<<<<< HEAD


Remove the line

esabol

Still need to get make test working.

esabol · 2023-08-10T15:48:21Z

expected/init_test.out.in

@@ -33,3 +33,4 @@ psql:pg_sphere.test.sql:8622: NOTICE:  argument type pointkey is only a shell
 psql:pg_sphere.test.sql:8628: NOTICE:  argument type pointkey is only a shell
 psql:pg_sphere.test.sql:8634: NOTICE:  argument type pointkey is only a shell
 psql:pg_sphere.test.sql:8640: NOTICE:  argument type pointkey is only a shell
+


Why did you add this blank line? I think it should be removed.

Take a look at the diff output at the bottom of the log here:
https://app.travis-ci.com/github/postgrespro/pgsphere/jobs/607714835

You need to change the line numbers in this file to match the results so that there are no differences.

esabol · 2023-08-10T15:48:56Z

expected/init_test_healpix.out.in

@@ -1,2 +1,3 @@
 psql:pg_sphere.test.sql:9207: NOTICE:  return type smoc is only a shell
 psql:pg_sphere.test.sql:9213: NOTICE:  argument type smoc is only a shell
+


Remove this blank line. In this file, change "9207" to "9221" and change "9213" to "9227".

esabol

Looks good to me! Thank you for your contribution and perseverance, @ggnmstr!

vitcpp · 2023-08-11T19:42:03Z

@ggnmstr Thank you for your contribution. I plan to merge PR #40 first, your PR will be the next. I guess, new differences may appear - one more iteration may be required to fix the tests. It seems to be a problem of init_test that should be redesigned in the future.

vitcpp · 2023-08-11T20:31:23Z

@ggnmstr Could you please do one more iteration - re-sync your branch with this repo, resolve conflicts and force-push your changes? Your PR will be the next. Thank you in advance!

ggnmstr · 2023-08-12T04:42:25Z

@vitcpp, Ok, I did it. The tests are OK, I guess, I just realized that I was typing "HEAPLIX" all the time and then wondering why is it not working on my PC

vitcpp · 2023-08-12T06:42:14Z

@ggnmstr Thank you very much!

ggnmstr force-pushed the poly_convex branch 2 times, most recently from d89edcf to a0be9d3 Compare August 3, 2023 09:40

vitcpp requested changes Aug 3, 2023

View reviewed changes

src/polygon.c Outdated Show resolved Hide resolved

src/polygon.c Outdated Show resolved Hide resolved

src/polygon.c Outdated Show resolved Hide resolved

ggnmstr force-pushed the poly_convex branch 3 times, most recently from b61c748 to 5d264e6 Compare August 4, 2023 03:59

ggnmstr force-pushed the poly_convex branch from 5d264e6 to c3b3cf7 Compare August 4, 2023 05:34

ggnmstr requested a review from vitcpp August 4, 2023 05:49

ggnmstr changed the title ~~add spoly_is_convex~~ Add spoly_is_convex Aug 5, 2023

ggnmstr force-pushed the poly_convex branch 3 times, most recently from 544f4f6 to 4570340 Compare August 6, 2023 04:32

esabol suggested changes Aug 6, 2023

View reviewed changes

ggnmstr force-pushed the poly_convex branch 5 times, most recently from 2620fa7 to a35e842 Compare August 6, 2023 07:22

esabol suggested changes Aug 6, 2023

View reviewed changes

ggnmstr force-pushed the poly_convex branch from a35e842 to 5012568 Compare August 6, 2023 15:51

This comment was marked as outdated.

Sign in to view

esabol suggested changes Aug 8, 2023

View reviewed changes

ggnmstr force-pushed the poly_convex branch 3 times, most recently from ff10fe4 to e740d2c Compare August 8, 2023 06:04

ggnmstr force-pushed the poly_convex branch from e740d2c to 6241f5a Compare August 8, 2023 09:44

ggnmstr force-pushed the poly_convex branch from 6241f5a to b11fea7 Compare August 8, 2023 14:21

vitcpp approved these changes Aug 9, 2023

View reviewed changes

ggnmstr force-pushed the poly_convex branch from b11fea7 to 5725967 Compare August 10, 2023 11:29

vitcpp requested changes Aug 10, 2023

View reviewed changes

ggnmstr force-pushed the poly_convex branch from 5725967 to 41c0326 Compare August 10, 2023 13:08

esabol suggested changes Aug 10, 2023

View reviewed changes

ggnmstr force-pushed the poly_convex branch 2 times, most recently from 006ad50 to e642169 Compare August 11, 2023 15:35

esabol approved these changes Aug 11, 2023

View reviewed changes

vitcpp approved these changes Aug 11, 2023

View reviewed changes

Add spoly_is_convex

d8470f8

ggnmstr force-pushed the poly_convex branch from e642169 to d8470f8 Compare August 12, 2023 04:07

esabol approved these changes Aug 12, 2023

View reviewed changes

vitcpp merged commit 6720f3a into postgrespro:master Aug 12, 2023

		@@ -1,2 +1,3 @@
		psql:pg_sphere.test.sql:9207: NOTICE: return type smoc is only a shell
		psql:pg_sphere.test.sql:9213: NOTICE: argument type smoc is only a shell

Add spoly_is_convex #43

Add spoly_is_convex #43

Conversation

ggnmstr commented Aug 2, 2023

vitcpp left a comment

Choose a reason for hiding this comment

vitcpp commented Aug 3, 2023 • edited Loading

dura0ok commented Aug 4, 2023 • edited Loading

ggnmstr commented Aug 4, 2023

dura0ok commented Aug 4, 2023

esabol commented Aug 4, 2023

dura0ok commented Aug 4, 2023

vitcpp commented Aug 5, 2023

esabol commented Aug 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

vitcpp commented Aug 7, 2023

ggnmstr commented Aug 8, 2023

dura0ok commented Aug 8, 2023

esabol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitcpp commented Aug 8, 2023

SELECT spoly_is_convex(NULL);

ggnmstr commented Aug 8, 2023

dura0ok commented Aug 8, 2023 • edited Loading

vitcpp left a comment

Choose a reason for hiding this comment

vitcpp commented Aug 9, 2023

ggnmstr commented Aug 9, 2023

Choose a reason for hiding this comment

esabol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol left a comment

Choose a reason for hiding this comment

vitcpp commented Aug 11, 2023

vitcpp commented Aug 11, 2023

ggnmstr commented Aug 12, 2023 • edited Loading

vitcpp commented Aug 12, 2023

vitcpp commented Aug 3, 2023 •

edited

Loading

dura0ok commented Aug 4, 2023 •

edited

Loading

esabol commented Aug 5, 2023 •

edited

Loading

dura0ok commented Aug 8, 2023 •

edited

Loading

ggnmstr commented Aug 12, 2023 •

edited

Loading